fix: improve getErrorDescFromResponse() performance#4110
Conversation
product-auto-label
bot
added
size: s
Summary of ChangesHello @logachev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a performance enhancement to the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a performance optimization for getErrorDescFromResponse by directly accessing GenericJson responses, which avoids expensive string serialization and re-parsing. However, there is a logic error in the field name used to extract the error message, and the implementation could be more robust regarding null safety and formatting.
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQueryRetryAlgorithm.java
Outdated
Show resolved
Hide resolved
logachev
force-pushed
the
kirl/retry_avoid_parsing
branch
from
0620ab3 to
8aa0a80
Compare
logachev
force-pushed
the
kirl/retry_avoid_parsing
branch
from
8aa0a80 to
9341a76
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request provides a significant performance improvement to getErrorDescFromResponse() by replacing an inefficient JSON serialization and parsing process with direct object access. The change is well-reasoned and correctly identifies that the error message structure being checked for is specific to Job objects. I have one suggestion to enhance the readability of the new implementation, making the null-checking logic more explicit.
| if (previousResponse instanceof Job) { | ||
| Job job = (Job) previousResponse; | ||
| ErrorProto error = job.getStatus() != null ? job.getStatus().getErrorResult() : null; | ||
| return error != null ? error.getMessage() : null; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
This is a great performance improvement. For better readability and to avoid potential maintenance issues with chained ternary operators, consider refactoring this block to use nested if statements. This makes the null-checking logic more explicit and easier to understand at a glance.
if (previousResponse instanceof Job) {
Job job = (Job) previousResponse;
com.google.api.services.bigquery.model.JobStatus status = job.getStatus();
if (status != null) {
ErrorProto error = status.getErrorResult();
if (error != null) {
return error.getMessage();
}
}
}
return null;
logachev
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
3 participants
getErrorDescFromResponse()is very inefficient. Running jfr profiler I noticed that ~55% of CPU-time running query that fetches ~300k rows/10 value each is spent in that function.Apparently, while it has a model object that inherits from
GenericJson, it serializes it & parses json again which is a lot of work when response represents page containing 10k rows.Judging based on the comment (and looking at available models) the only case when ResponseT contains
status.error_result.messageis forJobobject.Removed json serialization/parsing and scoped it down to
Jobobject.